Cross-compatible hook/middleware + "plugins" which are allowed to be hook and middleware#1284
Conversation
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Needs changelog + homepage docs, but lgtm! 🚀
…hook and middleware This one's largely aimed at extending a few parts of `otelriver` to be able to emit some additional useful metrics like time that it takes to lock jobs, number of jobs locked per batch, or any other arbitrary metrics we want to emit down the road. I previously had something similar back in #1203, but here we extract an isolated piece of it. This change does two things: * If either a hook sent to `Config.Hooks` implements a middleware or a middleware sent to `Config.Middleware` implements a hook, activate its alternate side as well. * Establish a new `Config.Plugins` that acts as a more generalized place where a hook/middleware can go. We define a plugin as this type: type Plugin interface { Hook Middleware } The reason for the first point is better backward compatibility. Notably, if I add a hook to `otelriver.Middleware`, I want it to be able to still work even if the user doesn't explicitly movie it from `Config.Middleware` to `Config.Plugins`.
Make hooks and middleware opt into `Plugin` through their default embedders and function helpers. Keep `Config.Plugins` as the unified registration path.
Merge the split lookup packages into one precomputed plugin lookup. Build the kind buckets up front and use the unified path throughout client startup and job execution.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e83736133
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| for _, hook := range hooks { | ||
| if plugin, ok := hook.(rivertype.Plugin); ok { | ||
| normalizedPlugins = append(normalizedPlugins, plugin) | ||
| } | ||
| } | ||
| for _, middleware := range middlewares { | ||
| if plugin, ok := middleware.(rivertype.Plugin); ok { | ||
| normalizedPlugins = append(normalizedPlugins, plugin) |
There was a problem hiding this comment.
NormalizePlugins now only carries registrations forward when they also satisfy rivertype.Plugin, but the public Config.Hooks, Config.Middleware, and JobArgsWithHooks APIs still accept plain rivertype.Hook/rivertype.Middleware values. In applications with custom hooks or middleware that manually implement IsHook/IsMiddleware (or embed both defaults without adding an unambiguous IsPlugin), those entries are silently dropped from every lookup, so their insert/work behavior stops running after this change; the original hook/middleware side should still be preserved even when the value is not a plugin.
Useful? React with 👍 / 👎.
| JobRow: job, | ||
| MiddlewareLookupGlobal: middlewarelookup.NewMiddlewareLookup(append(rivermiddleware.DefaultMiddleware(), w.config.Middleware...)), | ||
| PluginLookupByJob: pluginlookup.NewJobPluginLookup(), | ||
| PluginLookupGlobal: pluginlookup.NewPluginLookup(pluginlookup.NormalizePlugins(effectiveHooks, append(rivermiddleware.DefaultMiddleware(), effectiveMiddleware...), nil)), |
There was a problem hiding this comment.
In rivertest.Worker, effectiveHooks and effectiveMiddleware already expand cross-compatible registrations and plugins into both slices, but this line then normalizes both slices as independent plugin sources. When a test config uses a Config.Hooks item that also implements middleware, a Config.Middleware item that also implements hooks, or a Config.Plugins item implementing both, the same value is added twice to the plugin lookup and its hook/middleware runs twice under WorkJob, diverging from real Client behavior.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Followed this rabbit hole down, but trying to get into deduplication introduces a lot of edge cases (e.g. can't distinguish between zero-size pointers which are different structs, would have to try to dedup struct values in addition to struct pointers which gets questionable), so think it's best here to just require users only put any particular plugin in one place only.
…structs On the last point: Go cannot distinguish the same zero-sized pointer from distinct zero-sized pointers when their addresses coincide. The implementation favors preserving both registrations over silently dropping one.
It turns out that this is really hard to do in a way that's completely reliable. Even if we detect duplicate pointers, there's still room for a user to have used a duplicate struct. We also can't detect duplicate for zero-length structs. Since this is an edge case of an edge case, let's remove all plugin dedup/validation. If a user does this, they'll just have to live with the consequences.
|
Did a little rework on this one so that any existing hooks/middleware can be immediately moved to the new plugins section with no changes needed, and reworked the internal lookup paths so we unify hooks/middleware together ( (Oh and added changelog.) |
2 participants
This one's largely aimed at extending a few parts of otelriver to be able to
emit some additional useful metrics like time that it takes to lock jobs,
number of jobs locked per batch, or any other arbitrary metrics we want to emit
down the road. I previously had something similar back in #1203, but here we
extract an isolated piece of it.
Introduce
Config.Pluginsas the preferred place to install global extensions.A plugin may implement hooks, middleware, or both, allowing integrations to
participate in multiple lifecycle phases without being split across
configuration fields.
Config.HooksandConfig.Middlewareremain supported but are deprecated.Hooks that also implement middleware, and middleware that also implement hooks,
now activate both sides automatically.
Internally, replace the separate hook and middleware lookup paths with a
unified plugin lookup used by insertion, job execution, periodic jobs, and
rivertest. Existing defaults andBaseServiceinitialization continue towork through the unified path.
Hooks/middleware duplicated across multiple properties (
Config.Hooks,Config.Middleware,Config.Plugins) are deduplicated, so some effort shouldbe made to make sure any particular plugin is in only one place. For
simplicity, it's easiest just to put everything in
Config.Plugins.